-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: panic: close of closed channel #5939
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
for |
The usage of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
select { | ||
case <-ctx.Done(): | ||
return | ||
default: | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be logically the same but when ctx.Done()
is closed ctx.Err()
will return a non nil error. Just a personal pref on what's easier to read I guess.
select { | |
case <-ctx.Done(): | |
return | |
default: | |
} | |
if ctx.Err() != nil { | |
return | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this situation, for me, checking the error is confusing because we don't expect an error but a stop, and it will act like we are ignoring an error.
It's a personal pref too, but in this context, I prefer using Done
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go for it, either works. ✅
I used a global cancellable
context
becausecancel
is concurrency safe (and so can be called several times).The background of the problem is the fact of having go routines inside go routines inside go routines.
I think I will rewrite this section at some point.
To reproduce, I replaced
Next
with a non-existent methodFoo
.git clone https://github.com/NVIDIA/aistore.git cd aistore git checkout 7730e4290
Fixes #5938
Related to #5929, #5923